Bug-hunt round 33: Ctrl+C self-exit misdiagnosis, dead API, doc gaps - #50
Closed
REPPL wants to merge 5 commits into
Closed
Bug-hunt round 33: Ctrl+C self-exit misdiagnosis, dead API, doc gaps#50REPPL wants to merge 5 commits into
REPPL wants to merge 5 commits into
Conversation
record's <-ctx.Done() shutdown branch called finaliseOutputs on every
child unconditionally, unlike the sibling anyExit branch (fixed in
round 32), which excludes a self-exited child from the sweep. When a
recorder's done channel closed in the sub-millisecond window before
the interrupt reached the select, that recorder fell through to
finaliseOutputs and was either misdiagnosed via classifyMissingOutput
("stayed blocked on the permission prompt") when it captured nothing,
or silently exited 0 with a truncated recording presented as a clean
session when it left partial data.
Factor the pre-stopAll early-exit sampling into a shared
sampleEarlyExits helper used by both select cases, so a recorder that
exits on its own gets the same classifyRecorderExit diagnosis no
matter which case observes the exit.
Assisted-by: Claude:claude-sonnet-5
AGENTS.md listed go test ./... and go test -race ./... as two separate gates and said CI runs every gate above; CI and release.yml both run only the race-enabled line. Note the local-only line and narrow the CI claim to match (CLAUDE.md is a symlink to AGENTS.md, so one edit covers both). Assisted-by: Claude:claude-sonnet-5
Three gaps against the actual code and fixture: the words omission-reasons list did not mention that empty, whitespace-only, or invisible-only text is dropped (transcribe.go already documents this for segment text one row up); the timeline.jsonl field table had no Required column and did not state t's (and payload t1's) ±1e9-second bound, which ReadEntries enforces; and the utt-003 example trimmed the utterance text to start at "Now" while keeping t0 and the real word times from the untrimmed fixture, leaving the first shown word 1.6s after its own t0. Restore the example's opening words so t0 matches its first word again. Assisted-by: Claude:claude-sonnet-5
Validate had zero callers anywhere in the module — ingest uses the unexported validate/indexTimeline pair directly, and internal/review calls analyze.Load, not Validate. Being under internal/, it cannot have external consumers either. Drop it along with the now-unused errors import. Assisted-by: Claude:claude-sonnet-5
Assisted-by: Claude:claude-sonnet-5
Owner
Author
|
Closing: this branch ( Assisted-by: Claude:claude-sonnet-5 Generated by Claude Code |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Round 33 of the autonomous bug-hunt loop (state: issue #24).
Confirmed substantive (1)
record's<-ctx.Done()shutdown branch misdiagnosed (or silently swallowed) a recorder that self-exited just before the interrupt.internal/record/record.go's<-ctx.Done()case calledfinaliseOutputson every child unconditionally, unlike the siblinganyExitbranch (fixed in round 32, PR #48), which excludes a self-exited child from the sweep beforestopAllruns. When a recorder'sdonechannel closed in the (empirically measured, sub-millisecond) window before the interrupt reached the select, that recorder fell through tofinaliseOutputsexactly like a normally-stopped one:classifyMissingOutput's "stayed blocked on the permission prompt" narrative — disproved by its own exit, with the real exit status never surfaced (internal/record/tcc.go:116-129);audio.wav),finaliseOutputssaw usable data, appended no problem, andRunreturnednil— a truncated recording silently presented as a clean, complete session.Both failure shapes were empirically reproduced during verification (200/200 and 60/60 trials against the pre-fix code), and the defect directly contradicts the documented contract at
docs/reference/cli.md:127-130and the code's own stated invariant (internal/record/record.go, the comment above theanyExitbranch's exclusion logic).Fix: factored the pre-
stopAllearly-exit sampling both branches need into a sharedsampleEarlyExitshelper (internal/record/record.go), used by both select cases, so a recorder that exits on its own gets the sameclassifyRecorderExitdiagnosis regardless of which case observes the exit. Two new regression tests (internal/record/record_test.go:TestCtrlCDiagnosesRecorderThatSelfExitedWithNoOutput,TestCtrlCDiagnosesRecorderThatSelfExitedWithPartialOutput) were watched to fail against the pre-fix code and pass after.Confirmed nitpicks (4)
AGENTS.md(CLAUDE.mdis a symlink to it) listedgo test ./...as a CI gate distinct fromgo test -race ./...and claimed "CI runs every gate above";.github/workflows/ci.ymlandrelease.ymlin fact run only the race-enabled line. Corrected the claim; kept the plain line as a documented local-only command.docs/reference/session-directory.md'swordsomission-reasons row didn't mention that a word with empty, whitespace-only, or invisible-only text is also dropped (internal/transcribe/transcribe.go:529-537), unlike the equivalenttext-row rule documented one row up. Re-examined for present-day accuracy independent of origin — round 31 split-discarded a similarly framed finding on the different question of whether round 30 introduced fresh staleness; this round's two independent refuters both confirmed the row itself is incomplete regardless of when the behaviour was introduced.timeline.jsonlfield table had noRequiredcolumn and did not statet's (or a speech payload'st1's) ±1e9-second magnitude bound, whichinternal/timeline/timeline.go'sReadEntriesenforces — the same bound thetranscript.jsonltable documents fort0/t1three sections above.utt-003example trimmed the illustrated utterance text to start mid-sentence ("Now I expect…") while keeping the untrimmed fixture'st0and word times, leaving the first shown word 1.6s after its ownt0. Restored the example's opening words so they agree witht0again.internal/analyze/validate.go's exportedValidatefunction had zero callers anywhere in the module —Ingestuses the unexportedvalidate/indexTimelinepair directly, andinternal/reviewcallsanalyze.Load, notValidate. Being underinternal/, it cannot have external consumers either. Removed, along with the now-unusederrorsimport.Considered and refuted
timeline.jsonlfromanalyze's emitted analysis request via HTML-escaping (internal/session/session.go'sjsonlEncodervsinternal/analyze/emit.go's plainjson.Marshal): one refuter built HEAD and HEAD~1 and provedEmitRequest's output byte-identical across round 32's change — it re-decodes the timeline and re-marshals, soWriteJSONL's encoder choice never reaches it, and the asymmetry actually pre-dates round 32 viasession.SafeText. Split verdict, discarded.whisperx.go/whispercpp.goacceptingend < start): the downstreamt1→t0clamp is a documented, deliberate fallback (docs/reference/session-directory.md:70,internal/timeline/timeline.go), so no real misbehaviour survives..github/workflows/ci.yml's comment vs its actualBuildstep flags): real discrepancy, no behavioural consequence — the repo has no cgo-conditional source.docs/reference/cli.md:152's "ingest reads timeline.jsonl only" phrasing, read against ingest's ownfindings.jsonlverdict-guard scan: the same paragraph states that guard explicitly four sentences later; no reader is misled.docs/reference/session-directory.md'smanifest.jsonexample omitting optional fields present in the real fixture: both omitted fields are marked optional in the table directly above; not misleading.Verification
Every finding faced two independent adversarial refuters before being included above; only findings both refuters failed to kill were fixed. All four sweep dimensions (code, docs-vs-functionality, infrastructure, internal doc consistency) also surfaced several items that survived one but not both refuters — those are omitted per the loop's "when in doubt, discard" rule and not itemised here.
Gates
go build,gofmt -l .,go vet ./...,go test ./...,go test -race ./..., the pipeline smoke (merge+reportagainstexamples/sample-session), andsh -n install.sh && bash -n install.shall pass on the branch head..abcd/work/DECISIONS.mdgains the round's entry in this PR.